fix: Use default rcl allocator if allocator is std::allocator#3058
fix: Use default rcl allocator if allocator is std::allocator#3058jmachowinski merged 2 commits intoros2:rollingfrom
Conversation
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
@jmachowinski thanks for fixing this issue. (it's been a looooong time... 😓 )
the approach looks good to me. a couple of things.
- you happened to miss the DCO.
- a few deprecated warning (which are added by this PR) detected on else where.
btw, the custom allocator path is still left broken? i think this PR explicitly leaves the retyped_allocate/retyped_deallocate bugs in place for non-std::allocator users. it means users with custom allocators will still hit the same ASAN/valgrind issues. It would be worth a comment or deprecation warning on the non-default path as well?
|
@fujitatomoya This is the 'tick' version of this patch. It is also a first draft version (which I should have written in the pr...) as I did not wont to polish it if the patch got turned down again. #3054 was discussed in the last PMC meeting and was turned down for two reasons :
The big drawback of #3054 was that users of custom allocators would silently fall back to malloc / free for everything happening in rcl. I am not planning on fixing the code path for custom allocators, as the plan is to remove it altogether after the next release. I will mark it as deprecated though. |
jmachowinski
force-pushed
the
alloc_try2
branch
from
66eb056 to
1aa75b5
Compare
This fixes a bunch of warnings if using ASAN / valgrind on newer OS versions. It also fixed a real bug, as giving the wrong size on deallocate is undefined behavior according to the C++ standard. This version of the patch keeps the behavior for users that specified an own allocator the same and in therefore back portable. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski
force-pushed
the
alloc_try2
branch
from
1aa75b5 to
62ab1f6
Compare
|
Pulls: #3058
|
This commit adds the feature, that the user can now specify a method rcl_allocator_t get_rcl_allocator() on the given allocator. This method will be called if present, to get the rcl allocator. If the method is not present, the code will fall back to the old (and faulty) implementation and show a deprecation warning. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski
force-pushed
the
alloc_try2
branch
from
62ab1f6 to
449b653
Compare
|
Pulls: #3058
|
|
@jmachowinski any chance for this to be backported on Jazzy? BTW thanks a lot for working on this |
|
@jmachowinski we need to backport this to kilted? |
|
@Mergifyio backport kilted |
✅ Backports have been createdDetails
|
* fix: Use default rcl allocator if allocator is std::allocator This fixes a bunch of warnings if using ASAN / valgrind on newer OS versions. It also fixed a real bug, as giving the wrong size on deallocate is undefined behavior according to the C++ standard. This version of the patch keeps the behavior for users that specified an own allocator the same and in therefore back portable. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * feat: Provide a way to suppress the deprecation warning This commit adds the feature, that the user can now specify a method rcl_allocator_t get_rcl_allocator() on the given allocator. This method will be called if present, to get the rcl allocator. If the method is not present, the code will fall back to the old (and faulty) implementation and show a deprecation warning. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> --------- Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <j.machowinski@cellumation.com> (cherry picked from commit 9f79f40)
4 participants
This fixes a bunch of warnings if using ASAN / valgrind on newer OS versions. It also fixed a real bug, as giving the wrong size on deallocate is undefined behavior according to the C++ standard.
This version of the patch keeps the behavior for users that specified an own allocator the same and in therefore back portable.
Alternative to #3054
USED AI:
Yes, chatgpt to generate the template methods to check for the existence of a method on a class in c++17.